Quadlet - add full support for Symlinks #24018
Conversation
openshift-ci
bot
added
release-note
approved
ygalblum
force-pushed
the
allow-symlinks
branch
2 times, most recently
from
02f21f9
to
0cdab9f
Compare
cmd/quadlet/main.go
Outdated
| varExist, dirs := getDirsFromEnv(dirs) | ||
| if !varExist { | ||
| resolvedUnitDirAdminUser := resolveUnitDirAdminUser() |
There was a problem hiding this comment.
I this is would be cleaner written as if varExist {return dirs} so you do not have to indent the other logic
There was a problem hiding this comment.
You're right, fixed. With the addition of not returning dirs the if statement become even simpler
cmd/quadlet/main.go
Outdated
| } | ||
|
|
||
| func getDirsFromEnv() (bool, []string) { | ||
| func getDirsFromEnv(dirs map[string]struct{}) (bool, map[string]struct{}) { |
There was a problem hiding this comment.
Unlike other types, maps are behind a shred pointer and modified in place when passed around. As such it is not needed to return the same map back to the caller unless I am missing something.
There was a problem hiding this comment.
Yes, you are correct. Leftover from the previous implementation. Fixed
cmd/quadlet/main.go
Outdated
| nonNumericFilter := getNonNumericFilter(resolvedUnitDirAdminUser, systemUserDirLevel) | ||
| dirs = getRootlessDirs(dirs, nonNumericFilter, userLevelFilter) | ||
| } else { | ||
| dirs = getRootDirs(dirs, userLevelFilter) |
There was a problem hiding this comment.
same idea could be used here i think? early return.
There was a problem hiding this comment.
You're right. But, I have to say in the previous implementation the return inside the rootless statement kinda confused me. So, I rather have a single return when it's not really an early return but rather a different path
|
couple of changes most of which @Luap99 points out; else LGTM |
Use os.ReadDir recursively instead of filepath.WalkDir Use map instead of list to easily find looped Symlinks Update existing tests and add a more elaborate one Update the man page Signed-off-by: Ygal Blum <[email protected]>
ygalblum
force-pushed
the
allow-symlinks
branch
from
0cdab9f
to
133ea31
Compare
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 7aedb54
into
containers:main
Does this PR introduce a user-facing change?
Yes
Resolves: #23755